Add ard_categorical_max()
#244
Conversation
Unit Tests Summary 1 files 169 suites 1m 15s ⏱️ Results for commit 2f14d54. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit aa39b27 ♻️ This comment has been updated with latest results. |
|
Code Coverage SummaryDiff against mainResults for commit: c023a1a Minimum allowed coverage is ♻️ This comment has been updated with latest results |
|
Thanks @edelarua ! Can you help me understand the differences between this and the hierarchical function? Also, when we refer to ordered, does that mean ordered factors? |
This performs similarly to the hierarchical function but doesn't use hierarchies, so calculates event rates for flat tables. The For example, the grade rows in this table could be calculated with Honestly, we could probably simplify the hierarchical functions by implementing this upstream in |
|
Hmmm, before we merge this in, can we brainstorm together a bit? Either today or next week? |
|
Would another way to calculate these quantities be: ADAE |>
dplyr::slice_max(AESEV, n = 1, with_ties = FALSE, by = USUBJID) |>
cards::ard_categorical(
by = TRTA,
variables = AESEV,
denominator = ADSL |> dplyr::select(USUBJID, TRTA = ARM)
) |
| call = get_cli_abort_call() | ||
| ) | ||
| } | ||
| if (is_empty(denominator)) denominator <- data |
There was a problem hiding this comment.
since we're doing this, should we just make the denominator default value denominator = data in the function definition ? Then it will be more clear to users what the default is
| function(x) { | ||
| ard_categorical( | ||
| data = data |> | ||
| cards:::arrange_using_order(c(id, by, x)) |> |
There was a problem hiding this comment.
Unfortunately, CRAN won't allow the :::, so we'll have to copy the function into cardx. But please add a note that it's copied from cards and the reason we are using it (so we don't forget!)
| ard_categorical( | ||
| data = data |> | ||
| cards:::arrange_using_order(c(id, by, x)) |> | ||
| dplyr::slice_tail(n = 1L, by = all_of(c(id, intersect(by, names(denominator))))), |
There was a problem hiding this comment.
This part intersect(by, names(denominator)) is confusing me a bit. What if someone passes an integer as the denom? Then it will return NULL, and as a result, we would have dplyr::slice_tail(n = 1L, by = all_of(id)), but above we sorted by c(id, by, x) so the max value of x could have appeared in the first by level and not been sorted to the bottom. Right?
I am trying to think through the implications of this line.... 🤔 What if some specifies by=c("ARM", "SEX"), but the denominator only has 'SEX'? In the previous step, we've sorted by ID, then ARM, then SEX, the the variable. Then within ID and SEX, we're taking the last observation....Is that what we want? Should it be a requirement that the denom dataset has all the by variables when it is a data frame? (I am really not sure, so I am asking!) 😆
| fmt_fn = fmt_fn, | ||
| stat_label = stat_label | ||
| ) |> | ||
| list() |
There was a problem hiding this comment.
Since we're in an lapply() we don't need to pipe this into list(), right?
|
|
||
| # print default order of character variable levels --------------------------- | ||
| for (v in variables) { | ||
| if (is.character(data[[v]])) { |
There was a problem hiding this comment.
To be on the safe side, can we print this for all variables?
What changes are proposed in this pull request?
ard_categorical_max()to calculate categorical occurrence rates by maximum level per unique ID. (Add ARD function for counting patients #240)Closes #240
Pre-review Checklist (if item does not apply, mark is as complete)
usethis::pr_merge_main()ard_*()function was added, it passes the ARD structural checks fromcards::check_ard_structure().ard_*()function was added,set_cli_abort_call()has been set.ard_*()function was added and it depends on another package (such as,broom),is_pkg_installed("broom")has been set in the function call and the following added to the roxygen comments:@examplesIf do.call(asNamespace("cardx")$is_pkg_installed, list(pkg = "broom""))devtools::test_coverage()Reviewer Checklist (if item does not apply, mark is as complete)
devtools::test_coverage()When the branch is ready to be merged:
NEWS.mdwith the changes from this pull request under the heading "# cardx (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (seeNEWS.mdfor examples).